Skip to content

Balance integration fix #1982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Balance integration fix #1982

wants to merge 3 commits into from

Conversation

@faculerena
Copy link
Contributor

@faculerena faculerena commented Nov 9, 2023 •
edited
Loading

Summary

  • [N] y/n | Does it introduce breaking changes?
  • [N] y/n | Is it dependant on the specific version of cargo-contract or pallet-contracts?

Updated the default balance in integration tests.

Description

We updated the default balance in integration tests to be the same as in e2e-tests. This is not definitive, as we want opinions if this should be like this.

Checklist before requesting a review

  • My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

ascjones
ascjones reviewed Nov 13, 2023
View reviewed changes
crates/env/src/engine/off_chain/test_api.rs Outdated
// set up the funds for the default accounts
let substantial = 1_000_000;
// the 1_000_000_000 is the same value as in the e2e tests
let substantial = 1_000_000_000;
Copy link
Collaborator

@ascjones ascjones Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original 1_000_000 value was somewhat arbitrary anyway, so makes sense to change it to be the same as the substrate-contracts-node used for testsin.

Is it only the Alice account that is endowed with this amount? I see bob and charlie are still set to 1_000 below - does that match up with the endowed amounts for the e2e tests too? Seems kind of low.

Copy link
Contributor Author

@faculerena faculerena Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, i will check the behaviour and make it equal for all name accounts (at least). The accounts created with ::From([1;32]) should have a balance of 0 or the existential minimum?

@XianPaz XianPaz mentioned this pull request Nov 13, 2023
Open
@faculerena
Changed the default account balance to the testnet genesis default
8ec3b37 
This is the same value as substrate-contracts-node/node/src/chain_spec.rs `test_genesis` function
@faculerena
Copy link
Contributor Author

faculerena commented Nov 15, 2023 •
edited
Loading

I changed the value to be the same as the testnet genesis. This is the same value that this test gives in an e2e test.

  let alice_balance = client
      .balance(ink_e2e::account_id(ink_e2e::AccountKeyring::Alice))
      .await
      .expect("Failed to get account balance");

In this case, alice_balance is (1 << 60), and for each account in the keyring this is true too, except for one and two, that is 0.

@faculerena faculerena requested a review from ascjones November 15, 2023 16:16
@cmichi
Copy link
Collaborator

cmichi commented Nov 13, 2024

@faculerena Could you merge master into your PR? There are some conflicts that have to be resolved.

@cmichi
Copy link
Collaborator

cmichi commented Oct 14, 2025

Thanks for the work on this PR. It took a long while, but we now came to a decision that results in the closing of this PR. Some words on the reasoning and why it took so long:

The project moved out of Parity and with that came a period in which we didn't have funding and things were unclear. We resumed work a while ago to work on ink! v6; this is a migration away from WebAssembly and pallet-contracts to RISC-V and pallet-revive. We wrote down more context about this transition here.

While migrating to pallet-revive, we were uncertain how to handle the off-chain testing environment. We've now decided to remove it in favor of E2E tests against either a full node process or a sandboxed pallet-revive.

The reason for this decision is that the off-chain testing environment comes with a lot of maintenance costs and never fully reflected the on-chain behavior.

@cmichi cmichi closed this Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmichi cmichi Awaiting requested review from cmichi cmichi is a code owner

@SkymanOne SkymanOne Awaiting requested review from SkymanOne

@ascjones ascjones Awaiting requested review from ascjones

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@faculerena @cmichi @ascjones